-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add option to control airmass/perez enhancement factor in clearsky.ineichen #459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Some test failures are due to #481. |
This PR is going to end up accomplishing most of the remaining work for #394. To do includes:
|
I think this is close to ready to go. It needs someone to review it, especially the changes to The majority of the other changes are cleaning up the test suite so that it's not tightly coupled to this function. The The to do:
|
For what it's worth, I browsed through the changes and didn't notice any problems. Is it possible to have tests that compare outputs to the previous stable release, instead of hard coding lists of constants? |
Thanks @adriesse. Interesting idea to compare test results to previous stable releases. Maybe there is a framework/package for that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In clearsky.py
:
I recommend we remove the comment paragraphs at line 88 starting with "Dan's note..." and "Create the corrected TL..." This was developmental material in PVLib which was not included in the release.
We should revise the paragraph at line 104 to point to the Ineichen / Perez paper [1] as the reference. We should clarify that [2] implements a different version of the model in [1]. Suggest the following:
# ghi is calculated using either the equations in [1] by setting perez_enhancement=False (default behavior) or using the model in [2] by setting perez_enhancement=True.
@cwhanse I changed the comments. I'm not 100% sure I captured what you were going for. I think this is ready to merge. The test pass on all Travis CI configurations. The appveyor test failures are intermittent and unrelated. |
I think the comments are fine. My intent was to discard the no-longer relevant text from the Matlab file, and to clarify how this code implements the equations in those two Ineichen/Perez papers. Any comments from others before this is merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to ineichen
look good to me, I briefly glanced at some of the tests and mocker stuff, but not thoroughly. Thanks for making these changes.
See #435 for context.
git diff upstream/master -u -- "*.py" | flake8 --diff
and/or landscape.io linting service.docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.Examples of ineichen with and without the enhancement at a few latitudes, with simplied solis as a reference:
While the differences at higher latitudes are more noticeable, the differences at lower latitudes are still notable: a few % difference at solar noon, unphysical pedestals near the horizon.
Many tests failed when I changed the default, so still work to do on that.